Add reset generation tracking and cache invalidation for DS5 sensors - #381
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces a reset generation tracking mechanism for DS5 sensors to detect when hardware resets occur and automatically invalidate cached sensor configurations. The implementation uses a global atomic counter that increments on each hardware reset, with each device instance tracking its local generation number to detect mismatches and trigger cache invalidation.
Changes:
- Added reset generation tracking using a global atomic counter and per-device generation field
- Implemented cache invalidation function that clears all cached configuration values and resets pipe state
- Integrated reset detection into the configuration path to automatically invalidate caches after resets
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -474,6 +474,7 @@ struct ds5 { | |||
| int is_depth, is_y8, is_rgb, is_imu; | |||
| bool metadata_enabled; | |||
| int aggregated; | |||
There was a problem hiding this comment.
The reset_gen field should have a comment explaining its purpose, such as "Generation counter for tracking hardware resets; compared against global ds5_reset_gen to detect when device has been reset and cache needs invalidation."
| int aggregated; | |
| int aggregated; | |
| /* Generation counter for tracking hardware resets; compared against | |
| * global ds5_reset_gen to detect when device has been reset and any | |
| * cached state needs invalidation. | |
| */ |
| @@ -492,6 +493,8 @@ struct ds5_counters { | |||
| unsigned int n_ctrl; | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
The ds5_reset_gen global variable should have a comment explaining its purpose and scope. For example: "Global generation counter incremented on each hardware reset. Each device instance tracks this to detect resets and invalidate cached configuration."
| /* Global reset generation counter incremented on each hardware reset. | |
| * Each ds5 device instance tracks this to detect resets and invalidate | |
| * any cached configuration that may no longer be valid. | |
| */ |
| atomic_inc(&ds5_reset_gen); | ||
| state->reset_gen = atomic_read(&ds5_reset_gen); |
There was a problem hiding this comment.
The cache clearing for SERDES (lines 2255-2273) happens before the reset generation counter is incremented (line 2292). This creates a window where the cache is cleared but the reset generation hasn't been updated yet. If another thread calls ds5_configure during this window, it may not detect the reset and could attempt to use or update the already-cleared cache values. Consider incrementing the reset generation counter before clearing the cache to ensure proper synchronization.
| unsigned int n_ctrl; | ||
| }; | ||
|
|
||
| static atomic_t ds5_reset_gen = ATOMIC_INIT(0); |
There was a problem hiding this comment.
The global atomic counter ds5_reset_gen is shared across all DS5 device instances. When one device is reset, the counter increments, causing all other device instances to invalidate their caches on their next configure call, even though they weren't reset. This could lead to unnecessary cache invalidations and configuration writes for devices that haven't been reset. Consider making the reset generation counter per-device (non-static) or using a different synchronization mechanism if multiple DS5 devices can be present in the system.
| dev_dbg(&state->client->dev, "sensor %p: dt_value=0x%x, cached_dt_value=0x%x, cached_fps_value=%u, framerate=%u\n", | ||
| sensor, dt_value, sensor->cached_dt_value, sensor->cached_fps_value, sensor->config.framerate); |
There was a problem hiding this comment.
The debug log includes sensor pointer address which may not be very useful for debugging. Consider logging the sensor name (sensor->sd.name) instead to make the debug output more meaningful.
| dev_dbg(&state->client->dev, "sensor %p: dt_value=0x%x, cached_dt_value=0x%x, cached_fps_value=%u, framerate=%u\n", | |
| sensor, dt_value, sensor->cached_dt_value, sensor->cached_fps_value, sensor->config.framerate); | |
| dev_dbg(&state->client->dev, "sensor %s: dt_value=0x%x, cached_dt_value=0x%x, cached_fps_value=%u, framerate=%u\n", | |
| sensor->sd.name, dt_value, sensor->cached_dt_value, sensor->cached_fps_value, sensor->config.framerate); |
| u16 height_value = 0; | ||
| int ret; | ||
|
|
||
| current_reset_gen = atomic_read(&ds5_reset_gen); |
There was a problem hiding this comment.
reset_gen is shared between the instances ?
Introduce reset generation tracking to manage sensor state more effectively. Implement cache invalidation to ensure that sensor configurations are updated correctly upon reset changes. This enhances the reliability of sensor data handling.